-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add optional column alignment in write #117
base: main
Are you sure you want to change the base?
Conversation
It looks like a nice feature. Not my own style, but certainly that of many others I know. And I agree it helps for navigating huse namelists like the one you linked. Setting a minimum colwidth feels like a confusing and iterative exercise here, with one trying to guess what it should be to fit across all groups. Is that right?. Seems like one wants it to be "enough to align all values across all groups"? Or just a single group? Also, I have been moving away from adding arguments to the API for features like this, and towards setting properties in the I would also include |
I thought about automatically setting the column width, but I've decided it doesn't suit my purposes, as the gap is too large in MOM input.nml, and auto-width also runs the risk of a single-line change producing whitespace changes throughout the namelist, making it confusing to track namelist changes e.g. with git. Taking the width as a parameter allows the calling code to automatically set it if desired (e.g. how I implemented it here). Auto-fitting on a per-group basis reduces both those problems but would require a more complicated implementation. Agreed, I'm not sure about making it a property - I tried something like that years ago with the |
I don't recall what the problem was with adding There might be some objection to OO style here, but it feels more consistent to me to add this as a property. |
Yes, it's a formatting setting, the same as |
It's too bad the sorting idea got lost in #50, we should revisit it. |
I don't have an opinion either way re. argument vs. property, just so long as the functionality is available somehow to my application in |
Andrew, I apologize for letting this PR slip by. I recently made a change which will split strings which exceed the column limit: https://github.com/marshallward/f90nml/tree/string_split which does rework the header, so I expect it will disrupt this PR. I will merge that one today and then come back to this one. My initial thoughts:
Looking over my past comments, I don't think a |
BTW I am currently trying to merge this, so probably no need yet for you to work on it. |
great, thanks @marshallward |
91ac2a5
to
a6556fb
Compare
Hi @marshallward, just wondering if you'd consider this PR.
It adds an optional
colwidth
argument towrite
and associated functions that sets a minimum column width between the start of a variable name and the start of " = " in the output. This can make output much more readable, e.g. https://github.com/COSIMA/01deg_jra55_iaf/blob/ak-dev/ice/cice_in.nmlIt uses
ljust
, so if a variable name is too long it will simply push the " = " along a bit more (i.e. the variable name won't be trimmed - seehistory_deflate_level
in the link). The usual behaviour is retained with the defaultcolwidth=0
.